Is there anything we can do here in the case of both failing? Emit some events?

What would be the goal for emitting the event? Events are mostly just used by relayers I guess.

I think it would be worth logging something to INFO or DEBUG logs. Although I haven't seen SDK modules utilizing logging much.

What would be the goal for emitting the event?

Just some kind of signalling I guess, was just thinking aloud really. I think logging at WARN or ERROR here may be useful, but like you say, most sdk modules don't seem to do much logging.
Feels a little wrong to just silently no-op, that's my only concern.

Yeah, I know what you mean. I guess the problem is that even if we returned an error here we would simply dismiss it at the level above anyway, otherwise, we would err on OnAcknowledgementPacket which would throw away the ack for the ics20 transfer or w/e as well.

I think we're limited in what we can do here so my vote would be some warn/err logs.

Seems like a bug if we cannot refund to the original sender? Maybe we should panic

Hmm, maybe we should... originally @AdityaSripal had suggested to just no-op in the issue.

I guess the no-op is for the case that the other fees were distributed fine but this one errors. If we err or panic the other fees won't ever get distributed.

I think it would be worth adding a comment here explaining why this logic is necessary. I find it a bit confusing. 🤔

I think you need to separate these conditions as well. What happens if there is an err and refundAcc == receiver?

Maybe something like:

if err != nil {
    // fee distribution failed, try to refund to original incentivizer
    if bytes.Equal(receiver, refundAccAddress) {
        return err
    }
    
    // attempt to distribute fee to refund address
}

Could we add a test case for this case?

If there's an error and refundAcc == receiver then sending to the refundAcc has already failed, right?
That's why I combined the conditions using the logical AND.
The issues writes:

If we are trying to refund to original sender and that fails, then we need to just no-op and continue.

In this case, writeFn() is still called but there would be no state changes to commit (no-op). Do you think this would cause any problems?
I can adapt this tomorrow tho if we feel its better separated.

I'll look at adding a testcase 👍

Tests require some refactoring. In progress!

edit: oh man, just realised some of the same refactoring was merged yesterday in #1239

To be on the safe side I think it makes sense to ensure writeFn() doesn't get called. Otherwise we need to verify that no partial state transitions occurred in x/bank. It might assume state changes are discarded once the error is returned. Nice catch on the refundAcc == receiver transfer already occurring.

Sure, I'll adapt the logic to represent that. I had similar concerns wrt writeFn() but wasn't 100%. Thanks :)

I don't think we should be using HasBalance as it just ensures it is greater than or equal to.

Updated to be explicit.
Fetch the account balances below, pre fee distribution, then assert the required amounts are added.

I'll see if I can fix this test. Currently failing because we're relying on EscrowPacketFee and malleate() is called after this. If we call malleate() before escrowing the fees, then the RefundAddress (incentivizer) is the module account, which doesn't have balance.

Bit of a mouth full, but hope it makes sense.

The simplest solution is to mutate packetFee.RefundAddress in malleate() after the fees are initially escrowed and before DistributePacketFees is called 👍